Feature: Merge Expr and GenExpr
#1114
Conversation
Changed the type of _lhs and _rhs attributes in the ExprCons class from unspecified to 'object' to clarify their expected type and improve type safety.
Replaces the __init__ method with __cinit__ in the Variable class and updates the argument type to SCIP_VAR*.
Updated references from 'terms' to 'children' for Expr objects throughout Model methods to reflect changes in the Expr API. This ensures compatibility with the updated data structure and avoids errors when accessing expression terms.
Introduces _to_nodes methods for Expr, PolynomialExpr, and UnaryExpr to convert expressions into node lists for SCIP construction. Refactors Model's constraint creation to use the new node format, simplifying and clarifying the mapping from expression trees to SCIP nonlinear constraints.
Changed Expr from a Cython cdef class to a standard Python class for improved compatibility and maintainability. Removed cdef public dict children, as attribute is now managed in Python.
Zeroto521
force-pushed
the
issue/1074
branch
from
c13b346 to
8719b91
Compare
This reverts commit 28e6673.
Converted SumExpr, ProdExpr, and PowExpr from cdef classes to regular Python classes for improved compatibility and maintainability.
There was a problem hiding this comment.
Pull Request Overview
This PR merges the Expr and GenExpr classes into a unified expression system. The refactoring replaces the previous dual-system (polynomial Expr and general GenExpr) with a single hierarchy based on Expr, using specialized subclasses like PolynomialExpr, SumExpr, ProdExpr, and various function expressions (ExpExpr, LogExpr, etc.).
Key changes:
- Unified expression representation with
childrenreplacingterms Variableclass no longer inherits fromExpr- Simplified expression tree structure with improved type system
- Refactored constraint creation methods to use the new expression system
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pyscipopt/scip.pyi | Updated Variable class to remove inheritance from Expr |
| src/pyscipopt/scip.pxi | Modified Variable class structure and added operator overloading methods to delegate to expression system; refactored constraint creation methods to use children instead of terms |
| src/pyscipopt/scip.pxd | Updated Variable class declaration to remove Expr inheritance |
| src/pyscipopt/propagator.pxi | Simplified variable creation by removing unnecessary temporary variable |
| src/pyscipopt/expr.pxi | Complete rewrite of expression system replacing Expr/GenExpr duality with unified class hierarchy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Deleted the definition of the Expr class, which was not used in the code. This helps clean up the codebase and improves maintainability.
Changed ExprCons from a cdef class to a standard Python class and added type hints to the constructor parameters. This improves code readability and compatibility with Python tooling.
Moved Cython and C imports below standard library imports for better organization. Reformatted the addMatrixVar method signature and the addCons loop for improved readability and consistency.
Implements the __iadd__ method for PolynomialExpr, allowing in-place addition of polynomial expressions by updating child coefficients. Falls back to superclass behavior for non-polynomial operands.
|
Finish the base feature and function. And something needs to be done:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise TypeError("expr must be an Expr instance") | ||
| if lhs is None and rhs is None: | ||
| raise ValueError( | ||
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" |
There was a problem hiding this comment.
The error message is grammatically incorrect. It should be "Ranged ExprCons (with both lhs and rhs) isn't supported" or "Ranged ExprCons (with both lhs and rhs) is not supported" instead of "doesn't supported".
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" | |
| "Ranged ExprCons (with both lhs and rhs) is not supported" |
src/pyscipopt/scip.pxi
Outdated
| self = self.__add__(other) | ||
| return self |
There was a problem hiding this comment.
The __iadd__ implementation is incorrect. In Cython/Python, when self = self.__add__(other) is executed, the reassignment only affects the local variable self within the method, not the object itself. This means the in-place addition doesn't actually modify the Variable. While Variables are now immutable value objects (since they don't inherit from Expr), the __iadd__ should simply return the result of __add__ without the misleading assignment.
| self = self.__add__(other) | |
| return self | |
| return self.__add__(other) |
| def _evaluate(self, term): | ||
| self._checkStage("SCIPgetSolVal") | ||
| result = 1 | ||
| cdef _VarArray wrapper | ||
| wrapper = _VarArray(term.vartuple) | ||
| for i in range(len(term.vartuple)): | ||
| wrapper = _VarArray(term.vars) | ||
| for i in range(len(term.vars)): | ||
| result *= SCIPgetSolVal(self.scip, self.sol, wrapper.ptr[i]) | ||
| return result |
There was a problem hiding this comment.
The _evaluate method assumes that all children in expr.children are Term objects with a .vars attribute (line 1061). However, for non-polynomial expressions (like Expr({SinExpr(x): 1.0})), the children can be other Expr subclasses that don't have a .vars attribute, which will cause an AttributeError. The code should either check the type and handle non-Term children appropriately, or document that solution evaluation only works for polynomial expressions.
| m, x, y, z = model | ||
| expr = Expr() | ||
| assert expr.degree() == 0 | ||
| assert expr.degree() == float("inf") |
There was a problem hiding this comment.
The degree of an empty Expr() should be 0 (representing a zero constant), not float("inf"). An empty expression has no variables and no terms, which is effectively a constant zero. The float("inf") degree is reserved for non-polynomial expressions like exp(), log(), etc.
| assert expr.degree() == float("inf") | |
| assert expr.degree() == 0 |
| return Expr(self._remove_zero()) | ||
|
|
||
| def degree(self) -> float: | ||
| return max((i.degree() for i in self)) if self.children else float("inf") |
There was a problem hiding this comment.
The degree() method returns float("inf") for an empty expression, but this is inconsistent with the expected behavior. An empty Expr() should represent a zero constant and have degree 0, not infinity. The infinity degree should only apply to non-polynomial expressions (like those involving exp, log, sin, etc.). This affects the base implementation in line 244.
| return max((i.degree() for i in self)) if self.children else float("inf") | |
| return max((i.degree() for i in self)) if self.children else 0 |
| return ExprCons(self, lhs=other[CONST], rhs=other[CONST]) | ||
| return (self - other).__eq__(0) | ||
| elif isinstance(other, MatrixExpr): | ||
| return other.__ge__(self) |
There was a problem hiding this comment.
The __eq__ method in line 206 calls other.__ge__(self) when other is a MatrixExpr, but this should call other.__eq__(self) to maintain the equality constraint semantics. Using __ge__ would incorrectly create a greater-than-or-equal constraint instead of an equality constraint.
| return other.__ge__(self) | |
| return other.__eq__(self) |
|
|
||
|
|
||
| class ExprCons: | ||
| """Constraints with a polynomial expressions and lower/upper bounds.""" |
There was a problem hiding this comment.
The docstring says "Constraints with a polynomial expressions" but should say "Constraints with polynomial expressions" (without "a" before "polynomial"). Also note that ExprCons can now handle non-polynomial expressions too (like exp, log, etc.), so the docstring is inaccurate and should be updated to reflect that it handles general expressions.
| """Constraints with a polynomial expressions and lower/upper bounds.""" | |
| """Constraints with general expressions and lower/upper bounds.""" |
| relevant_value = relevant_value[:-1] # removing % | ||
|
|
||
| if _is_number(relevant_value): | ||
| if isinstance(relevant_value, Number): |
There was a problem hiding this comment.
The check isinstance(relevant_value, Number) will always be False because relevant_value is a string extracted from the file (see line 12083 and 12117). Strings are never instances of Number. This appears to be a logic error introduced when replacing the old _is_number() helper function. The code should use a try/except block or recreate the _is_number() function to check if the string can be converted to a number.
|
Thanks for your work @Zeroto521 !! This is something that should be very very well tested before merging. We're also in the middle of a SCIP meeting, so I can bring this up with the guys and we can quickly take a high level look. |
Replaces use of variable pointers for hashing and equality in the Term class with Python's built-in hash function on the sorted variable tuple. This simplifies the implementation and improves consistency. Also updates degree check in node conversion.
Introduced __slots__ to Expr, ProdExpr, and PowExpr classes to reduce memory usage and improve attribute access performance.
Type annotations were added to _normalize, __le__, and __ge__ methods in ExprCons. Error handling was improved by moving type checks earlier in __le__ and __ge__ to ensure arguments are Numbers before proceeding.
The __next__ method in Expr was unnecessary since iteration is handled by __iter__. This simplifies the class and avoids potential confusion.
Introduces the _first_child() method to Expr for cleaner child access. Updates PowExpr and UnaryExpr to use _first_child() in __repr__ and normalization logic, improving code readability and consistency.
Replaces use of '+=' for list concatenation with 'append' and 'extend' methods for clarity and consistency in node list construction across Term, Expr, PolynomialExpr, and UnaryExpr classes. This improves readability and ensures uniform handling of node lists.
Replaces integer initialization with ConstExpr for the result in PolynomialExpr's power operation to ensure correct expression type handling.
Renamed the static method _is_sum to _is_SumExpr for clarity and updated all references. Improved multiplication logic to handle sum expressions and self-multiplication cases more explicitly.
Changed Expr and ExprCons classes to cdef for performance and added public attributes. Updated Model methods to consistently use ExprCons type annotations and parameter names, improving type safety and clarity in constraint creation and addition.
Converted Term to a cdef class for Cython optimization, added explicit type annotations to methods, and removed runtime type checks in __mul__ and __eq__ for improved performance and clarity.
Refactored the _normalize method to directly filter out zero-valued children, removing the separate _remove_zero helper function for simplicity.
Operator overloads in the Variable class now delegate to MonomialExpr.from_var(self) instead of self.to_expr(). The to_expr() method was removed, simplifying the code and making the delegation explicit.
Renamed internal methods from _to_nodes to _to_node across Term, Expr, PolynomialExpr, and UnaryExpr classes for consistency. Updated logic to handle coefficient application and node construction more robustly. Adjusted Model class in scip.pxi to use the new method name.
Introduces a degree() method to the Variable class, returning the degree of the variable using MonomialExpr. This enhances the API for users needing variable degree information.
Replaced direct iteration over children.items() with iteration over self or self.children and explicit indexing in several methods of Term, Expr, PolynomialExpr, and UnaryExpr. This improves consistency and leverages custom __getitem__ implementations for coefficient access.
Refactors the Variable.__iadd__ method to delegate in-place addition to MonomialExpr.from_var(self).__iadd__(other), ensuring correct behavior and consistency with other arithmetic operations.
Moved the static method _is_SumExpr to the end of the Expr class and updated its signature to use Cython type annotation. Also refactored variable naming in the __add__children method for clarity and removed unnecessary blank lines.
Implements __iadd__ for ConstExpr and MonomialExpr to support in-place addition with other polynomial expressions. Also refactors _first_child to _fchild and updates references for consistency.
2 participants
Close to #1074. This is a breaking change. We can release the 5.7.0 version first.